Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement signWithPasskey #194

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Implement signWithPasskey #194

wants to merge 10 commits into from

Conversation

heliuchuan
Copy link
Contributor

Description

Test Plan

Related Links

src/api/aptos.ts Show resolved Hide resolved
src/bcs/deserializer.ts Outdated Show resolved Hide resolved
src/bcs/serializer.ts Outdated Show resolved Hide resolved
src/transactions/transactionBuilder/transactionBuilder.ts Outdated Show resolved Hide resolved
src/transactions/transactionBuilder/transactionBuilder.ts Outdated Show resolved Hide resolved
src/core/crypto/webauthn.ts Outdated Show resolved Hide resolved
src/internal/passkeysBrowser.ts Outdated Show resolved Hide resolved
export async function signWithPasskey(args: {
aptosConfig: AptosConfig;
credentialId: HexInput;
publicKey: HexInput;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you already have the passkey / other information, or do you need to pull this out of elsewhere?

src/internal/passkeysBrowser.ts Outdated Show resolved Hide resolved
src/core/crypto/p256.ts Outdated Show resolved Hide resolved
src/core/crypto/p256.ts Outdated Show resolved Hide resolved
Comment on lines +73 to +82
export type ClientDataJSON = {
type: string;
challenge: string;
origin: string;
crossOrigin?: boolean;
tokenBinding?: {
id?: string;
status: "present" | "supported" | "not-supported";
};
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to differentiate between clientDataJSON and CollectedClientData (see passkey-rs) and keep clientDataJSON as type ArrayBuffer, consistent with the webauthn spec

Suggested change
export type ClientDataJSON = {
type: string;
challenge: string;
origin: string;
crossOrigin?: boolean;
tokenBinding?: {
id?: string;
status: "present" | "supported" | "not-supported";
};
};
export type ClientDataType = "webauthn.get" | "webauthn.create" | "payment.get"
export type CollectedClientData = {
type: ClientDataType;
challenge: string;
origin: string;
crossOrigin?: boolean;
[key: string]: any
};
export type ClientDataJSON = Uint8Array;

<meta charset="UTF-8" />
<link rel="icon" type="image/svg+xml" href="/vite.svg" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Vite + React + TS</title>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<title>Vite + React + TS</title>
<title>Passkeys Aptos React Demo</title>

@@ -0,0 +1,35 @@
{
"name": "passkey-ts",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"name": "passkey-ts",
"name": "passkey-react-demo",

const BOB_INITIAL_BALANCE = 100;
const TRANSFER_AMOUNT = 7777777;
const COIN_STORE = "0x1::coin::CoinStore<0x1::aptos_coin::AptosCoin>";
const config = new AptosConfig({ network: Network.LOCAL });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a useState for the network so the user can toggle between Devnet / Localnet and Testnet / Mainnet in the future?

// Copyright © Aptos Foundation
// SPDX-License-Identifier: Apache-2.0

import { PublicKeyCredentialCreationOptionsJSON, RegistrationResponseJSON } from "@simplewebauthn/server/esm/deps";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import { PublicKeyCredentialCreationOptionsJSON, RegistrationResponseJSON } from "@simplewebauthn/server/esm/deps";
import type { PublicKeyCredentialCreationOptionsJSON, RegistrationResponseJSON } from "@simplewebauthn/server/esm/deps";

Comment on lines +66 to +102
verifySignature(args: { message: HexInput; signature: Secp256r1Signature }): boolean {
const { message, signature } = args;

const msgHex = Hex.fromHexInput(message).toUint8Array();
const sha3Message = sha256(msgHex);
const rawSignature = signature.toUint8Array();
return p256.verify(rawSignature, sha3Message, this.toUint8Array());
}

/**
* Verifies a signed data with a public key
*
* @param args.message message
* @param args.signature The signature
* @returns true if the signature is valid
*/
verifyWebAuthnSignature(args: { message: HexInput; signature: WebAuthnSignature }): boolean {
const { message, signature } = args;

if (!(signature.paar.signature.signature instanceof Secp256r1Signature)) {
throw new Error("Attestation signature is not a Secp256r1Signature");
}

// Check challenge
const { challenge } = signature.getCollectedClientData();

const messageBase64URLString = bufferToBase64URLString(Hex.fromHexInput(message).toUint8Array());
if (challenge !== messageBase64URLString) {
return false;
}

// Get verification data.
const verificationData = signature.getVerificationData();

// Verify the the signature is the signed verification data.
return this.verifySignature({ message: verificationData, signature: signature.paar.signature.signature });
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may be better for this to be just the plain verification method (i.e., not for webauthn signatures). See secp256r1_ecdsa_sigs.rs. And then the webauthn verify / verifyWebAuthnSignature methods can be added to PartialAuthenticatorAssertionResponse to be consistent with aptos-core

}
}

export class PartialAuthenticatorAssertionResponse {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, I think it may make more sense to move the verify methods here to be consistent with aptos-core but lmk if you feel differently. Here's webauthn.rs for reference.

<h1>Passkeys Demo</h1>
{passkeyAddr ? <p className="text-wrap">{"Your address: " + passkeyAddr}</p> : null}
{passkeyAddr ? (
<a

Check warning

Code scanning / CodeQL

Potentially unsafe external link Medium

External links without noopener/noreferrer are a potential security risk.
{passkeyAddr ? <p className="text-wrap">{"Your address: " + passkeyAddr}</p> : null}
{passkeyAddr ? (
<a
href={`https://explorer.aptoslabs.com/account/${passkeyAddr}/transactions?network=${currentNetwork}`}

Check warning

Code scanning / CodeQL

DOM text reinterpreted as HTML Medium

DOM text
is reinterpreted as HTML without escaping meta-characters.
@alex4506
Copy link

Some projects depend on passkey, looking forward to completion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants